SWC-6669: migrate legacy EntityViewScopeEditor to react component #5309
Conversation
jinjunoh
commented
Feb 28, 2024
jinjunoh
commented
- create respective components for EntityViewScopeEditorModal (props, view, impl, test)
- modify existing ScopeWidgetTest
| return props; | ||
| } | ||
| } | ||
| // TODO: create EntityViewScopeEditorModalWidget, View, and IMPL |
There was a problem hiding this comment.
| // TODO: create EntityViewScopeEditorModalWidget, View, and IMPL |
| private String entityId; | ||
| private EntityViewScopeEditorModalProps.Callback onCancel; | ||
| private EntityViewScopeEditorModalProps.Callback onUpdate; | ||
| boolean open; |
There was a problem hiding this comment.
I don't think you need to store open because you are always using the passed parameter.
| boolean open; |
| EntityContainerListWidget viewScopeWidget, editScopeWidget; | ||
| SynapseAlert synAlert; | ||
| EntityContainerListWidget viewScopeWidget; | ||
| EntityViewScopeEditorModalWidget editEntityViewScopeModalWidget; // TODO: replace edit scope widget with the react compoenent widget & fix corresponding test |
There was a problem hiding this comment.
| EntityViewScopeEditorModalWidget editEntityViewScopeModalWidget; // TODO: replace edit scope widget with the react compoenent widget & fix corresponding test | |
| EntityViewScopeEditorModalWidget editEntityViewScopeModalWidget; |
| String entityId, | ||
| EntityViewScopeEditorModalProps.Callback onUpdate, | ||
| EntityViewScopeEditorModalProps.Callback onCancel, | ||
| boolean open |
There was a problem hiding this comment.
for consistency with SqlDefinedEditorModalWidget.java, recommend removing open from the configure signature and making the initial value of the prop false.
| () -> { | ||
| editEntityViewScopeModalWidget.setOpen(false); | ||
| }, | ||
| true |
There was a problem hiding this comment.
You don't need this if you change the signature as suggested above.
If you did keep this, then the setOpen below is redundant
| } | ||
|
|
||
| @Test | ||
| public void testConstruction() { |
There was a problem hiding this comment.
| public void testConstruction() { | |
| public void testConfigure() { |
| ); | ||
| onCancelArgumentCaptor.getValue().run(); | ||
| verify(mockEntityViewScopeEditorModalWidget).setOpen(false); | ||
| } |
There was a problem hiding this comment.
This would pass if you had a bug where onCancel ended up being misconfigured to call what was supposed to be the onUpdate prop. Just to make sure the correct callback was called, you should make sure fireEvent was never called.
| } | |
| verify(mockEventBus, never()).fireEvent(any(EntityUpdatedEvent.class)); | |
| } |
| onUpdateArgumentCaptor.capture(), | ||
| onCancelArgumentCaptor.capture(), |
There was a problem hiding this comment.
Since you aren't using the captors, you can just do
| onUpdateArgumentCaptor.capture(), | |
| onCancelArgumentCaptor.capture(), | |
| any(EntityViewScopeEditorModalProps.Callback.class), | |
| any(EntityViewScopeEditorModalProps.Callback.class), |
There was a problem hiding this comment.
Looks good, need an SRC release that includes Sage-Bionetworks/synapse-web-monorepo#756 to merge
